Skip to content

fix(nanno): preserve user-requested field order in AnnotationSourceTSV#402

Merged
holmeso merged 4 commits intonanno_multi_genefrom
copilot/sub-pr-400-again
Mar 3, 2026
Merged

fix(nanno): preserve user-requested field order in AnnotationSourceTSV#402
holmeso merged 4 commits intonanno_multi_genefrom
copilot/sub-pr-400-again

Conversation

Copy link

Copilot AI commented Feb 27, 2026 •
edited
Loading

Description

AnnotationSourceTSV.getHeaderNameAndPositions previously used a HashMap, which made iteration order non-deterministic when building fieldNames/fieldPositions. This could mismatch the user-requested field order and emptyRecordResult.

This PR keeps the fix focused on field ordering in nanno:

  • AnnotationSourceTSV.java: changed HashMap to LinkedHashMap in getHeaderNameAndPositions so insertion order follows the requested fields order.
  • AnnotateTest.java: updated jsonInputsTSV expected output to match deterministic user-requested order (aaref,HGVSc_VEP,HGVSp_VEP).

Per review feedback, earlier Executor.java changes were removed from this PR and will be handled in a separate PR.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Targeted qannotate nanno tests for this PR scope:

  • AnnotationSourceTSVTest.extractFieldsFromRecord
  • AnnotationSourceTSVTest.getHeaderNameAndPositions
  • AnnotateTest.jsonInputsTSV

Are WDL Updates Required?

No WDL updates are required.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI mentioned this pull request Feb 27, 2026
Open
@holmeso
fix: use LinkedHashMap in getHeaderNameAndPositions to preserve user-…
79cc951 
…requested field order

Co-authored-by: holmeso <7066552+holmeso@users.noreply.github.com>
Copilot AI changed the title [WIP] Address feedback on reporting variants in multiple genes fix(nanno): preserve user-requested field order in AnnotationSourceTSV Feb 27, 2026
@holmeso
Copy link
Contributor

holmeso commented Feb 27, 2026

@copilot this causes a failure in a qannotate test

@holmeso
fix: remove --enable-preview, fix Executor classpath quoting, update …
d646599 
…test expectations

- Revert --enable-preview from build.gradle (List.getFirst/getLast are standard Java 21, not preview)
- Revert --enable-preview from AnnotateTest.java Executor call
- Update jsonInputsTSV expected value to match user-requested field order (aaref,HGVSc_VEP,HGVSp_VEP) with LinkedHashMap
- Fix Executor.java: use getJavaPath() instead of literal "java", remove shell-style quotes from classpath (ProcessBuilder does not use a shell), switch array/jvmArgs constructors to ProcessBuilder

Co-authored-by: holmeso <7066552+holmeso@users.noreply.github.com>
Copy link
Author

Copilot AI commented Feb 27, 2026

@copilot this causes a failure in a qannotate test

Fixed in commit d646599. There were two separate issues:

  1. AnnotateTest.jsonInputsTSV – the expected value was written for the old HashMap (non-deterministic) ordering. Updated to "aaref=M\tHGVSc_VEP=c.1A>C\tHGVSp_VEP=p.Met1?" which matches the user-requested field order "aaref,HGVSc_VEP,HGVSp_VEP" preserved by the LinkedHashMap.

  2. AnnotateTest.endToEnd / endToEndSnpEff – pre-existing bugs in Executor.java: the 2-arg constructor passed a shell-quoted classpath ("\"...\") to ProcessBuilder, which doesn't use a shell, so the literal quote characters prevented the JVM from loading classes. Also, the "java" literal was replaced with getJavaPath() (derived from java.home) for robustness. List.getFirst()/getLast() are standard Java 21 features — no --enable-preview is needed.

@holmeso
Copy link
Contributor

holmeso commented Mar 3, 2026

@copliot please take out the changes to Executor - I will put them in a future PR

@holmeso
Copy link
Contributor

holmeso commented Mar 3, 2026

@copilot please review the previous comment

@holmeso
revert: remove Executor.java changes from this PR
f3e112a 
Co-authored-by: holmeso <7066552+holmeso@users.noreply.github.com>
Copy link
Author

Copilot AI commented Mar 3, 2026 •
edited
Loading

@copliot please take out the changes to Executor - I will put them in a future PR

Done in f3e112a — all Executor.java changes have been removed from this PR so they can be handled separately. No UI changes in this update (backend-only).

@copilot please review the previous comment

Reviewed and applied in f3e112a: this PR now excludes Executor.java changes and keeps only the field-order fix scope. No UI changes in this update (backend-only).

@holmeso holmeso marked this pull request as ready for review March 3, 2026 01:48
@holmeso holmeso merged commit cbf60aa into nanno_multi_gene Mar 3, 2026
1 check passed
@holmeso holmeso deleted the copilot/sub-pr-400-again branch March 3, 2026 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@holmeso holmeso holmeso approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@holmeso holmeso

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@holmeso